Skip to content

feat(tls): allow password users to connect over TLS without a client certificate - #1283

Merged
levkk merged 5 commits into
pgdogdev:mainfrom
bhargavtheertham-cb:feat/per-user-tls-client-certificate-required
Jul 30, 2026
Merged

feat(tls): allow password users to connect over TLS without a client certificate#1283
levkk merged 5 commits into
pgdogdev:mainfrom
bhargavtheertham-cb:feat/per-user-tls-client-certificate-required

Conversation

@bhargavtheertham-cb

@bhargavtheertham-cb bhargavtheertham-cb commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Problem

Setting tls_client_ca_certificate makes client certificates mandatory for every TLS connection, because rustls' default client verifier denies anonymous clients. On a listener serving both mTLS users (identity) and password users (password/password_hash), that leaves the password users unable to negotiate TLS at all — they get a certificate_required alert, so their only working option is a plaintext connection:

$ psql "host=... user=some_password_user sslmode=require"
psql: error: connection to server at "..." failed:
      SSL error: tlsv13 alert certificate required

$ psql "host=... user=some_password_user sslmode=disable"
psql (17.7)          # works, unencrypted

PgDog can't resolve this during the handshake, which completes before the startup packet carrying user= arrives — at that point it has no idea who is connecting. So the choice is currently all-or-nothing for every connection on the listener.

Worth noting the current requirement doesn't actually protect those users: they're already reachable unauthenticated over plaintext. It only forces the legitimate ones onto the unencrypted path.

Change

Build the verifier with allow_unauthenticated() so a certificate is verified when presented but its absence no longer fails the handshake, and move the requirement to authentication, where the user is known.

A new per-user tls_client_certificate_required defaults to true and is enforced only when a client CA is configured and the client connected over TLS:

  • without a client CA, no certificate is ever requested (with_no_client_auth), so requiring one could never be satisfied — enforcing it unconditionally would reject every user in every deployment that doesn't configure client CAs;
  • plaintext connections stay governed by tls_client_required.
[[users]]
name = "app"
database = "db"
password_hash = "SCRAM-SHA-256$4096:..."
tls_client_certificate_required = false   # TLS, no client cert

Backwards compatibility

Behavior is unchanged at the default:

Client Before After
TLS, no cert, client CA set rejected (TLS alert) rejected (NoClientCertificate)
TLS, no cert, opted out rejected authenticates over TLS
TLS, valid cert verified verified, unchanged
TLS, untrusted cert rejected at handshake rejected at handshake
Plaintext governed by tls_client_required unchanged
No client CA configured cert never requested unchanged

The one visible difference at default is that a certless client is now rejected after the handshake rather than during it, so the error is a Postgres auth failure instead of a TLS alert. That also means slightly more work per rejected connection — happy to gate the verifier change behind a config flag instead if you'd prefer to keep the early rejection.

mTLS is unaffected

Identity enforcement never lived in the handshake — client/mod.rs already requires stream.tls_identity() == Some(identity) for any user with a configured identity, and a certless client gets None, so it still fails with NoIdentity. An entry with neither identity nor password still fails closed via the existing NoPasswordConfig guard. Certificates that are presented are verified against the CA bundle exactly as before; only the empty-certificate case changes.

Testing

  • cargo fmt --all -- --check and cargo clippy --all-targets -- -D warnings clean
  • cargo test -p pgdog-config — 83 passed
  • New: client_cert_verifier_allows_anonymous_clients asserts offer_client_auth() stays true while client_auth_mandatory() becomes false; two config tests cover the unset and opted-out cases

Setting `tls_client_ca_certificate` makes client certificates mandatory
for every TLS connection, because rustls' default client verifier denies
anonymous clients. On a listener serving both mTLS users (`identity`) and
password users, that leaves the password users unable to negotiate TLS at
all: they get a `certificate_required` alert, so their only working option
is a plaintext connection.

PgDog cannot decide this during the handshake, which completes before the
startup packet carrying `user=` arrives. At that point it has no idea who
is connecting.

Build the verifier with `allow_unauthenticated()` so a certificate is
verified when presented but its absence no longer fails the handshake, and
move the requirement to authentication, where the user is known. A new
per-user `tls_client_certificate_required` defaults to true and is enforced
only when a client CA is configured and the client connected over TLS:

  - without a client CA no certificate is ever requested, so requiring one
    could never be satisfied;
  - plaintext connections stay governed by `tls_client_required`.

Behavior is unchanged at the default. A user connecting over TLS without a
certificate is still rejected, now with `NoClientCertificate` at the auth
layer rather than a TLS alert. mTLS is unaffected: users with `identity`
are still rejected by the existing `tls_identity()` comparison, and an
entry with neither identity nor password still fails closed via
`NoPasswordConfig`. Certificates that are presented are verified against
the CA bundle exactly as before; only the empty-certificate case changes.

Track certificate presence on `Stream` separately from `tls_identity`,
which is `None` both when no certificate was sent and when one was sent
without a dNSName SAN or Subject CN.

Co-Authored-By: Claude <noreply@anthropic.com>
@bhargavtheertham-cb
bhargavtheertham-cb marked this pull request as draft July 29, 2026 22:28
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

bhargavtheertham-cb and others added 3 commits July 30, 2026 09:53
Extract the four-condition guard into `missing_required_client_certificate`
so the logic is stated once and can be exercised directly, and cover the
cases Codecov flagged:

  - a truth table over the guard, including each way it must NOT fire
  - `NoClientCertificate` is an error and renders its message
  - plain and dev_null streams report no client certificate

Also shorten the doc comments added in the previous commit.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
The unit tests cover the pieces but nothing drives a real handshake
through to authentication, because `Client::login` needs a live TLS
stream and a populated cluster registry.

The mTLS integration suite already has both, so add the two cases this
change introduces to it: a password user that opts out connects over TLS
without a certificate, and one that doesn't opt out is still rejected.
Both share a listener with the existing identity users, which is the
arrangement the feature exists to allow.

Co-Authored-By: Claude <noreply@anthropic.com>
@bhargavtheertham-cb
bhargavtheertham-cb marked this pull request as ready for review July 30, 2026 15:08
Comment thread pgdog/src/frontend/client/mod.rs Outdated
/// Only meaningful when a client CA is configured, since otherwise no certificate
/// is ever requested, and only over TLS, since plaintext is governed by
/// `tls_client_required`.
fn missing_required_client_certificate(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you're better off passing in the original structs by reference, that way you can be sure the right bools are in the right place.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, you can make it a struct, so it's less likely to make a typo:

struct MissingParams {
  client_ca_configured: bool,
  is_tls: bool,
  // ...
}

@levkk levkk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Happy to merge as-is, but let me know if you'd want to refactor that 4 boolean method.

@bhargavtheertham-cb bhargavtheertham-cb changed the title feat: per-user tls_client_certificate_required feat(tls): allow password users to connect over TLS without a client certificate Jul 30, 2026
Four positional booleans made it possible to transpose `required` and
`presented` and silently stop enforcing the requirement, since only
`presented` is negated. Named fields make that wrong on its face.

The test loses its positional comment and now varies one field at a time
from a rejected baseline, so each case states which condition it is
exercising.

Co-Authored-By: Claude <noreply@anthropic.com>
@bhargavtheertham-cb

Copy link
Copy Markdown
Contributor Author

Took the struct suggestion — thanks, it was a real hole rather than just a style point.

Because the body is a flat && and only presented is negated, most transpositions were harmless, but swapping required and presented inverted the check: a user who owed a certificate and sent none evaluated to false and would have been let through. Silently, and only in that one combination.

} else if (ClientCertificateCheck {
    client_ca_configured,
    is_tls: stream.is_tls(),
    required: cluster.tls_client_certificate_required(),
    presented: stream.tls_client_certificate(),
})
.rejected()
{

I went with the params struct over passing &General/&Stream/&Cluster for one reason: constructing a real Stream needs a live TLS handshake, so that version couldn't be unit tested and the truth table would have had to go. The struct keeps it trivially constructible.

The test also reads better now — it varies one field at a time from a rejected baseline, so each case names the condition it's exercising instead of relying on a // client CA, is_tls, required, presented comment above four bare booleans.

@levkk
levkk merged commit e67ef2f into pgdogdev:main Jul 30, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants